Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove copy and move constructors from AxisDirection #3972

Merged
merged 1 commit into from
Nov 29, 2023

Conversation

jjimenezshaw
Copy link
Contributor

Recently it took me a few hours and the help from @hernando to find out the reason of a failure in my code.

Let's say I am trying to make this function

        auto axis = [&] (const std::string& name, const std::string& abbr, AxisDirection dir) {
            return CoordinateSystemAxis::create(
                PropertyMap().set(IdentifiedObject::NAME_KEY, name), abbr,
                dir, UnitOfMeasure::METRE);
        };

Currently it is assigning some garbage to the direction because it is passed by copy, and not by reference. I was calling it with AxisDirection::EAST, that naïvely I supposed it was an enum or enum class.
(the solution is passing the direction by reference... but the compiler was not helping me. The consequence was a crash later in the execution, not a compile error).

The implementation of the create function takes the pointer of the direction (pay attention to the & in &directionIn), that has a "wrong" value if it was copied. The consequence was a later segmentation fault when serializing to JSON.

CoordinateSystemAxisNNPtr CoordinateSystemAxis::create(
    const util::PropertyMap &properties, const std::string &abbreviationIn,
    const AxisDirection &directionIn, const common::UnitOfMeasure &unitIn,
    const MeridianPtr &meridianIn) {
    auto csa(CoordinateSystemAxis::nn_make_shared<CoordinateSystemAxis>());
    csa->setProperties(properties);
    csa->d->abbreviation = abbreviationIn;
    csa->d->direction = &directionIn;
    csa->d->unit = unitIn;
    csa->d->meridian = meridianIn;
    return csa;
}

The easy solution I implement in this PR is deleting the copy and move constructors / assignments operator, so the compiler it not allowing me that wrong operation.

That opens the question: why is it taking a pointer? I find that obscure. But probably changing that implementation may break the API, and would be a lot of work.

@rouault
Copy link
Member

rouault commented Nov 29, 2023

why is it taking a pointer? I find that obscure.

I believe the reason was to make comparisons to the constant easier, but probably the best solution would have to implement the operator ==().
Why not an enum/enum class ? Because those can't have methods on them. That said, we could likely have used them and utility functions to get a string from them, or return an instance from a string.
Sorry for the inconvenience... Your fix is very reasonable.

@rouault rouault merged commit ce39a39 into OSGeo:master Nov 29, 2023
22 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants